Skip to content

Rename the health check endpoint to /healthz#467

Merged
davidfowl merged 1 commit intomainfrom
davidfowl/healthz
Nov 2, 2023
Merged

Rename the health check endpoint to /healthz#467
davidfowl merged 1 commit intomainfrom
davidfowl/healthz

Conversation

@davidfowl
Copy link
Copy Markdown
Contributor

Fixes #447

@davidfowl
Copy link
Copy Markdown
Contributor Author

After this poll, I'm sure everyone will change this endpoint 😄

@brendandburns
Copy link
Copy Markdown
Contributor

brendandburns commented Oct 26, 2023

As I discussed w/ David, I'm team /healthy and /alive

@mitchdenny
Copy link
Copy Markdown
Member

+1 for healthz given the broader cloud native ecosystem is invested in this convention.

Copy link
Copy Markdown
Member

@IEvangelist IEvangelist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤣 at the /healthz...


// All health checks must pass for app to be considered ready to accept traffic after starting
app.MapHealthChecks("/readiness");
app.MapHealthChecks("/healthz");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
app.MapHealthChecks("/healthz");
app.MapHealthChecks("/healthy");


// All health checks must pass for app to be considered ready to accept traffic after starting
app.MapHealthChecks("/readiness");
app.MapHealthChecks("/healthz");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
app.MapHealthChecks("/healthz");
app.MapHealthChecks("/healthy");


// All health checks must pass for app to be considered ready to accept traffic after starting
app.MapHealthChecks("/readiness");
app.MapHealthChecks("/healthz");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
app.MapHealthChecks("/healthz");
app.MapHealthChecks("/healthy");

@DamianEdwards
Copy link
Copy Markdown
Member

@davidfowl are we doing this for preview.1 still?

@davidfowl
Copy link
Copy Markdown
Contributor Author

Yep, I will go with /health and /alive

@IEvangelist
Copy link
Copy Markdown
Member

Yep, I will go with /health and /alive

Why not /healthy? It's more aligned with /alive if that's a preference, but /health feels disjointed from /alive. If you're healthy, you're alive, but they're not dually synonymous. You could be alive, but unwell... Whereas, /health could be anything, what does this actually report? It is a bit, or some status object? Is it an enum with three possible values; Healthy, Degraded, and Unhealthy? If the latter is the case, then I'd assume that we wouldn't want /alive as I'd expect that to be either true or false.

When you say "and", are you using both, as in multiple routes that resolve with the same check? Just curious about this naming convention. I agree with @brendandburns on this one, but still a bit conflicted.

@davidfowl
Copy link
Copy Markdown
Contributor Author

davidfowl commented Oct 31, 2023

/health does a deep health check and checks all dependencies. It'll eventually also return detailed health status (it'll return a single result today, healthy, unhealthy, degraded)
/alive does a self-check without checking dependencies.

@davidfowl davidfowl enabled auto-merge (squash) November 2, 2023 18:39
@davidfowl davidfowl merged commit f9eb4b7 into main Nov 2, 2023
@davidfowl davidfowl deleted the davidfowl/healthz branch November 2, 2023 19:04
@davidfowl
Copy link
Copy Markdown
Contributor Author

/backport to release/8.0-preview1

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 2, 2023

Started backporting to release/8.0-preview1: https://github.com/dotnet/aspire/actions/runs/6737243070

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 2, 2023

@davidfowl backporting to release/8.0-preview1 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Patch format detection failed.
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 2, 2023

@davidfowl an error occurred while backporting to release/8.0-preview1, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

davidfowl added a commit that referenced this pull request Nov 2, 2023
davidfowl added a commit that referenced this pull request Nov 2, 2023
joperezr added a commit that referenced this pull request Nov 14, 2023
@danmoseley danmoseley added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Nov 16, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider changing the health check endpoint to /healthz

6 participants